-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add UI for Value Tracking #51974
Add UI for Value Tracking #51974
Conversation
return false; | ||
} | ||
|
||
var selectedSymbol = _threadingContext.JoinableTaskFactory.Run(() => GetSelectedSymbolAsync(textSpan, document, cancellationToken)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we talked to the platform folks about it being possible to have an async command handler so we don't need to use JTF for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this to just RunAsync, similar to https://github.com/madskristensen/AsyncToolWindowSample/blob/master/src/Commands/ShowToolWindow.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I like this a lot better. It being fire-and-forget seems fine compared to blocking the caller on async work.
private readonly ValueTrackingTreeViewModel _viewModel; | ||
|
||
// Needed for VSSDK003 | ||
// See https://github.com/Microsoft/VSSDK-Analyzers/blob/main/doc/VSSDK003.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make sure that we eventually change the tool window to be async. Synchronous tool window construction is surprisingly expensive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, plus async tool windows are not difficult to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I fixed this. I was close to having it, didn't realize I should just not block on window show. I followed https://github.com/madskristensen/AsyncToolWindowSample/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could anyone tell me what this feature is about? It sounds like something I'm interested in one of my analyzers. Thanks to a kind soul in advance! :) |
Thanks Andrew! I will check the source code, it looks useful for my scenario :-) |
I'm going to merge this, but feel free to leave more feedback and I'll address in a separate PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto-approval
This adds the start of the UI for value tracking. Builds off of #51898